Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

move chart to subdirectory #2

Merged
merged 9 commits into from
Mar 13, 2024
Merged

move chart to subdirectory #2

merged 9 commits into from
Mar 13, 2024

Conversation

kgrubb
Copy link
Contributor

@kgrubb kgrubb commented Mar 13, 2024

Description

To follow a similar format to helm's create option, this moves the chart into a subdirectory. It also simplifies the image config.

@kgrubb kgrubb self-assigned this Mar 13, 2024
Copy link
Contributor

@maksym-iv-elf maksym-iv-elf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but we may change the name in the templates a bit.
By default the only place where {{ include "helm_foobar.name" . }} is used is helpers, in

{{/*
Selector labels
*/}}
{{- define "helm_foobar.selectorLabels" -}}
app.kubernetes.io/name: {{ include "helm_foobar.name" . }}
app.kubernetes.io/instance: {{ .Release.Name }}
{{- end }}

And everywhere else {{ include "helm_foobar.fullname" . }} is used.

And we may want to add to the helpers and use {{ include "netobserve.serviceAccountName" . }} (from standard)

{{/*
Create a default fully qualified app name.
We truncate at 63 chars because some Kubernetes name fields are limited to this (by the DNS naming spec).
If release name contains chart name it will be used as a full name.
*/}}
{{- define "netobserve.fullname" -}}
{{- if .Values.fullnameOverride }}
{{- .Values.fullnameOverride | trunc 63 | trimSuffix "-" }}
{{- else }}
{{- $name := default .Chart.Name .Values.nameOverride }}
{{- if contains $name .Release.Name }}
{{- .Release.Name | trunc 63 | trimSuffix "-" }}
{{- else }}
{{- printf "%s-%s" .Release.Name $name | trunc 63 | trimSuffix "-" }}
{{- end }}
{{- end }}
{{- end }}

@@ -7,4 +7,4 @@

# Misc
tmp.*
charts/**
charts/netobserv/charts/**
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
charts/netobserv/charts/**
charts/netobserv/**

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that would ignore all of the templates 😱

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But charts/netobserv/charts/** does not seem to exist

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is where the helm dep update and helm dep build write to

charts/netobserv/values.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@maksym-iv-elf maksym-iv-elf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kgrubb kgrubb merged commit 42911a8 into main Mar 13, 2024
2 checks passed
@kgrubb kgrubb deleted the move-chart-to-subdir branch March 13, 2024 21:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants